Repository navigation
Conversation
Signed-off-by: somaz <genius5711@gmail.com>
|
@himsngh @praveenrewar — gentle ping on this one. It fixes a Worth flagging: none of the test workflows have ever executed against it. 8 of the 9 runs on the head sha sit at For a concurrency fix that's exactly the signal worth having. Could one of you approve the workflow run? Marking it ready for review on 20 Jul didn't re-dispatch them. |
The tests now use t to check that the reader iterated at least one result, and the AppsForRef writer adds and removes a second App, so the set it writes to is never empty. With a single CPU the old writer left the reader seeing only empty sets in about 2 of 100 runs, and the test then passed without exercising the race. Signed-off-by: somaz <genius5711@gmail.com>
|
Thanks for merging develop in. The lint run that followed flagged my new race tests, and fa32fda fixes them. While fixing that I found the AppsForRef test could pass without exercising the race, because its writer sometimes left the reader with only empty sets, so the writer now adds and removes a second App and both tests check that the reader iterated something. |
|
Hi @joaopapereira, the lint failures from the run after your develop merge are fixed in fa32fda. The workflows on that head are waiting for approval to run, since the earlier approval expired. Could you approve them and take a look when you have time? |
What this PR does / why we need it:
AppRefTracker.AppsForRefandRefsForAppreturned the internal map byreference. Callers such as
SecretHandler.enqueueAppsForUpdateandConfigMapHandler.enqueueAppsForUpdateiterate that map after the tracker lockis released, so a concurrent
ReconcileRefs/RemoveAppFromAllRefswriting thesame map crashes kapp-controller with
fatal error: concurrent map iteration and map write. This reproduces on large clusters with thousands ofnamespaces/secrets under continuous reconciliation.
Both methods now return a shallow copy of the RefKey set while holding the lock,
so callers iterate a private map and never race the writers. The two callers
only iterate the result (read-only), so returning a copy is behavior-preserving.
Which issue(s) this PR fixes:
Fixes #1843
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Added two regression tests in
pkg/reftracker/ref_tracker_race_test.go(concurrent read+iterate against a concurrent writer, for both
AppsForRefandRefsForApp). Before the fix they crash with the reported fatal error; after itthey pass, including under
-race.Validation run locally:
go test ./pkg/reftracker/... -racepasses (existing + new tests)gofmt/go vetcleanReview Checklist: